Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Placeholders not vertically centered in text inputs #4874

Merged
merged 1 commit into from
Sep 1, 2021

Conversation

PrashantMangukiya
Copy link
Contributor

Corrected Placeholders not vertically centered in text inputs. @chiragsalian kindly review pull request, please message me if any information missing from my side. I closed previous pull request #4823 because I forgot to sign commit with gpg.

Details

Basically when there is no focus it shows ExpensiTextInputLabel.js component. We need to update top value for expensiTextInputLabel style within src/styles/styles.js file. i.e. change top: 14 to top: 16 as under:

expensiTextInputLabel: {
...
    top: 16,
...
},

So now we moved inactive label default position 2 pt down, So to keep the active label (i.e. when focused) at the same position we have to set ACTIVE_LABEL_TRANSLATE_Y = -12 (at present it is -10) within src/components/ExpensiTextInput/BaseExpensiTextInput.js (line 12)

Fixed Issues

$ #4675

Tests

  1. Run app on any platform
  2. Check text input placeholder on Login screen, now it will show text input placeholder vertically centered when no input text.
  3. For after login screen, Text input with no value will show placeholder vertically centered where text input used.

QA Steps

  1. Login screen text input. When there is no focus on text input, placeholder shows vertically centered now. (previously it was vertically off centered)
  2. After login any text input. When there is no focus on text input, placeholder shows vertically centered now. (previously it was vertically off centered)

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

4675-Web

Mobile Web

4675-mobileWeb

Desktop

macOsDesktop

iOS

4675-iOS

Android

4675-Android7

@PrashantMangukiya PrashantMangukiya requested a review from a team as a code owner August 27, 2021 12:18
@MelvinBot MelvinBot requested review from cead22 and removed request for a team August 27, 2021 12:18
@PrashantMangukiya
Copy link
Contributor Author

Hi, @cead22, As soon as you get time kindly review my pull request 4874. This is my first pull request, if so anything missing then please message me. I will provide needed information as soon as possible. Thank you.

@cead22
Copy link
Contributor

cead22 commented Aug 30, 2021

@PrashantMangukiya code looks good, but please include screenshots showing active text inputs and their labels like you did in the other PR for completeness

@PrashantMangukiya
Copy link
Contributor Author

Hi @cead22 Attached is screenshots having text input value.

Web

4675-Web-Input

4675-Web-Input-inner

Mobile Web

4675-MobileWeb-Input

Desktop

4675-macOsDesktop-inner

iOS

4675-IOS-Input

Android

4675-Android7-2

4675-Android-3

@cead22 cead22 merged commit b3cb84e into Expensify:main Sep 1, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Sep 1, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Sep 2, 2021

🚀 Deployed to staging by @cead22 in version: 1.0.91-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@botify
Copy link

botify commented Sep 3, 2021

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-10. 🎊

@OSBotify
Copy link
Contributor

OSBotify commented Sep 3, 2021

🚀 Deployed to production by @roryabraham in version: 1.0.92-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants